HellPot NextGen #163
HellPot NextGen #163
Conversation
…zche While I do think all bots enjoy Nietzche (who doesn't?), I think we should take a stance to educate them. What better way than to be able to choose from any book! Personal suggestions include: - The Sorrows of Young Werther by Goethe - Any political manifesto - The Declaration of Independence etc. etc.
This removes globals from `heffalumpt/`, which are hard to reason about, easy to get wrong, and should not be created if they are never used. A possible drawbacks is if you would create multiple new defaults, but this should never be the case.
This prints the short variants (like `-c` for `--config`) in the help. Also fixes bug where only `-h` flag works, not `--help`.
additionally: - fix `-c` flag - make main package test better
|
Mixing and matching configuration sources seems to be working well. (Also pictured in screenshot: Grimoire test) Screenshot |
| break | ||
| } | ||
| n += copy(p[n:], w3) | ||
| n += copy(p[n:], "\n") | ||
| if time.Now().UnixNano()%10 == 0 { |
There was a problem hiding this comment.
Haven't run any profiling, but a quick glance makes me think about how much impact this has (but this is a great idea!). This is a call in a very hot loop so it makes me wonder if we can do this cheaper?
Also it might be a good idea to reorder to have the most common case first, but again measure first make GitHub review comments later might be a better plan...
There was a problem hiding this comment.
I 100% agree that profiling is necessary to determine the cost here. My initial instinct is that there are few things faster than the the time_now exec for a hot path like this that needs (some level of) entropy. but i agree this definitely needs to be explored and proven. Maybe just a simple i++ counter + modulo would be better.
the relevant (linux runtime) assembly is below, but there are some bitwise operations (in Go) that follow it's execution as well; at least for the UnixNano method.
// Copyright 2021 The Go Authors. All rights reserved.
// Use of this source code is governed by a BSD-style
// license that can be found in the LICENSE file.
//go:build !faketime
#include "go_asm.h"
#include "go_tls.h"
#include "textflag.h"
#define SYS_clock_gettime 228
// func time.now() (sec int64, nsec int32, mono int64)
TEXT time·now<ABIInternal>(SB),NOSPLIT,$16-24
MOVQ SP, R12 // Save old SP; R12 unchanged by C code.
MOVQ g_m(R14), BX // BX unchanged by C code.
// Set vdsoPC and vdsoSP for SIGPROF traceback.
// Save the old values on stack and restore them on exit,
// so this function is reentrant.
MOVQ m_vdsoPC(BX), CX
MOVQ m_vdsoSP(BX), DX
MOVQ CX, 0(SP)
MOVQ DX, 8(SP)
LEAQ sec+0(FP), DX
MOVQ -8(DX), CX // Sets CX to function return address.
MOVQ CX, m_vdsoPC(BX)
MOVQ DX, m_vdsoSP(BX)
CMPQ R14, m_curg(BX) // Only switch if on curg.
JNE noswitch
MOVQ m_g0(BX), DX
MOVQ (g_sched+gobuf_sp)(DX), SP // Set SP to g0 stack
noswitch:
SUBQ $32, SP // Space for two time results
ANDQ $~15, SP // Align for C code
MOVL $0, DI // CLOCK_REALTIME
LEAQ 16(SP), SI
MOVQ runtime·vdsoClockgettimeSym(SB), AX
CMPQ AX, $0
JEQ fallback
CALL AX
MOVL $1, DI // CLOCK_MONOTONIC
LEAQ 0(SP), SI
MOVQ runtime·vdsoClockgettimeSym(SB), AX
CALL AX
ret:
MOVQ 16(SP), AX // realtime sec
MOVQ 24(SP), DI // realtime nsec (moved to BX below)
MOVQ 0(SP), CX // monotonic sec
IMULQ $1000000000, CX
MOVQ 8(SP), DX // monotonic nsec
MOVQ R12, SP // Restore real SP
// Restore vdsoPC, vdsoSP
// We don't worry about being signaled between the two stores.
// If we are not in a signal handler, we'll restore vdsoSP to 0,
// and no one will care about vdsoPC. If we are in a signal handler,
// we cannot receive another signal.
MOVQ 8(SP), SI
MOVQ SI, m_vdsoSP(BX)
MOVQ 0(SP), SI
MOVQ SI, m_vdsoPC(BX)
// set result registers; AX is already correct
MOVQ DI, BX
ADDQ DX, CX
RET
fallback:
MOVQ $SYS_clock_gettime, AX
SYSCALL
MOVL $1, DI // CLOCK_MONOTONIC
LEAQ 0(SP), SI
MOVQ $SYS_clock_gettime, AX
SYSCALL
JMP retThere was a problem hiding this comment.
What we could do is to add some source of entropy at startup (or at the start of each request; one call to a random() is cheap), and the do modulo on that in some way. Any constant would make it a bit too predictable I think
There was a problem hiding this comment.
I agree, and that is a good idea for sure.
I think we should implement that idea in some form or another but I find myself hesitant to actually pull the trigger until we have a proper measuring apparatus set up so we can even rank these implementation concepts.
That's finding itself higher and higher on the to-do list it seems
|
Great effort, really nice! Don't have that much time to sit and make a review right now, but fun to see great changes! |
|
I still find myself really frustrated with the configuration system and ironing out all of the idiosyncrasies, even with Koanf vs Viper. I've been toying with the idea of just doing the toml ourselves. I've been bashing my head against writing the decoder part of it the last few days, but the encoder works well enough (not that that's the important part though). If there's anything I've learned while doing software development professionally, it's when to stop trying to use overly complex or just simply annoying dependencies that have been the source of endless bugs. It usually ends up that the best course of action is to instead just write the functionality yourself. To the untrained it may sound like an unnecessary time sink, but one has to take into account the amount of time/money/unbridled rage caused by the dependencies in question. The scales tend to start tipping in a different direction after such considerations. I digress, but it's an idea anyway. We'll see how it goes. |
| os.Exit(2) | ||
| } | ||
|
|
||
| for defaultKey, defaultVal := range Defaults.val { |
There was a problem hiding this comment.
Noting (for myself even): I was doing this here because of some problematic behavior when it comes to order of initialization and overrides when it comes to CLI vs defaults vs environment variables vs toml config
Bumps [github.com/knadh/koanf/providers/file](https://github.com/knadh/koanf) from 0.1.0 to 1.0.0. - [Release notes](https://github.com/knadh/koanf/releases) - [Commits](knadh/koanf@v0.1.0...v1.0.0) --- updated-dependencies: - dependency-name: github.com/knadh/koanf/providers/file dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <[email protected]>
Overview
I accidentally a whole
coca cola bottleHellPotThis really is essentially a rewrite.
The only thing that remains untouched more or less is the heffalump core.This should resolve countless idiosyncrasies in configuration behavior.
Improvements
New Features
strings.Containsmatch functionalityNew features by
are scheduled to hitch a ride, pending further review and testing.
Slimfast
Further slimming of dependencies
Issues resolved